Fix S1854 FP: Throw should connect to outer catch by mary-georgiou-sonarsource · Pull Request #9528 · SonarSource/sonar-dotnet

mary-georgiou-sonarsource · 2024-07-16T12:45:35Z

sebastien-marichal · 2024-07-16T15:49:09Z

analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs

                AddPredecessorsOutsideRegion(finallyBlock);
            }
        }
+        if (block.IsEnclosedIn(ControlFlowRegionKind.Catch) && block.Successors.FirstOrDefault(x => x.Semantics == ControlFlowBranchSemantics.Rethrow) is { })


Suggested change

if (block.IsEnclosedIn(ControlFlowRegionKind.Catch) && block.Successors.FirstOrDefault(x => x.Semantics == ControlFlowBranchSemantics.Rethrow) is { })

if (block.IsEnclosedIn(ControlFlowRegionKind.Catch) && block.Successors.FirstOrDefault(x => x.Semantics == ControlFlowBranchSemantics.Rethrow) is not null)

It's probably an FN for T0007.

sebastien-marichal

LGTM!
We discussed it together and it seems good for me.
I only added a comment about is not null usage

Tim-Pohlmann

LGTM only cosmetic suggestions.

Tim-Pohlmann · 2024-07-18T14:12:44Z

analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs

+    {
+        var tryRegion = block.EnclosingRegion(ControlFlowRegionKind.TryAndCatch).NestedRegion(ControlFlowRegionKind.Try);
+        foreach (var catchBlock in tryRegion.ReachableHandlers()
+                                            .Where(x => x.Kind == ControlFlowRegionKind.Catch && x != block.EnclosingRegion && x.FirstBlockOrdinal > block.Ordinal)


Suggested change

.Where(x => x.Kind == ControlFlowRegionKind.Catch && x != block.EnclosingRegion && x.FirstBlockOrdinal > block.Ordinal)

.Where(x => x.Kind == ControlFlowRegionKind.Catch && x.FirstBlockOrdinal > block.Ordinal)

The middle condition is already covered by the last condition.

Tim-Pohlmann · 2024-07-18T14:18:44Z

analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs

+        foreach (var catchBlock in tryRegion.ReachableHandlers()
+                                            .Where(x => x.Kind == ControlFlowRegionKind.Catch && x != block.EnclosingRegion && x.FirstBlockOrdinal > block.Ordinal)
+                                            .SelectMany(x => x.Blocks(Cfg)))


I would separate this into an extra variable. foreach with multiple lines is always awkward to read.

Tim-Pohlmann · 2024-07-18T14:28:43Z

...ts/SonarAnalyzer.Test/LiveVariableAnalysis/RoslynLiveVariableAnalysisTest.TryCatchFinally.cs

+    {
+        var code = """
+            var value = 100;
+            try{


Tim-Pohlmann · 2024-07-18T14:34:45Z

analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs

                AddPredecessorsOutsideRegion(finallyBlock);
            }
        }
+        if (block.IsEnclosedIn(ControlFlowRegionKind.Catch) && block.Successors.FirstOrDefault(x => x.Semantics == ControlFlowBranchSemantics.Rethrow) is { })


Suggested change

if (block.IsEnclosedIn(ControlFlowRegionKind.Catch) && block.Successors.FirstOrDefault(x => x.Semantics == ControlFlowBranchSemantics.Rethrow) is { })

if (block.IsEnclosedIn(ControlFlowRegionKind.Catch) && block.Successors.Any(x => x.Semantics == ControlFlowBranchSemantics.Rethrow))

sonarqubecloud · 2024-07-18T14:51:31Z

Quality Gate passed for 'Sonar .NET Java Plugin'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

sonarqubecloud · 2024-07-18T14:56:29Z

Quality Gate passed for 'SonarAnalyzer for .NET'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

pavel-mikula-sonarsource · 2024-07-19T06:34:42Z

analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs

                AddPredecessorsOutsideRegion(finallyBlock);
            }
        }
+        if (block.IsEnclosedIn(ControlFlowRegionKind.Catch) && block.Successors.Any(x => x.Semantics == ControlFlowBranchSemantics.Rethrow))


What about Throw semantics? All the UTs have throw, but throw new Excetion("Whaaat?", ex) should behave the same

fixed here #9530
Thanks @sebastien-marichal

pavel-mikula-sonarsource · 2024-07-19T06:36:33Z

analyzers/src/SonarAnalyzer.CFG/LiveVariableAnalysis/RoslynLiveVariableAnalysis.cs


+    private void BuildBranchesNestedCatchRethrow(BasicBlock block)
+    {
+        var reachableHandlers = block.EnclosingRegion(ControlFlowRegionKind.TryAndCatch).NestedRegion(ControlFlowRegionKind.Try).ReachableHandlers();


Doesn't ReachableHandlers also connect back to it's own Catch in this case? Or more precisely, all neighboring catches too? That would be wrong

@pavel-mikula-sonarsource I'm after asking for x.FirstBlockOrdinal > block.Ordinal) - all the catch with greater ordinal that the one I'm in.
Isn't that working?

var reachableHandlers = block.EnclosingRegion(ControlFlowRegionKind.TryAndCatch).NestedRegion(ControlFlowRegionKind.Try).ReachableHandlers(); foreach (var catchBlock in reachableHandlers.Where(x => x.Kind == ControlFlowRegionKind.Catch && x.FirstBlockOrdinal > block.Ordinal).SelectMany(x => x.Blocks(Cfg))) {}

Also not sure what you mean neighbouring catches :/

Try Catch Ex As IOException Throw 'You are here, with your block.Ordinal Catch Ex As FormatException 'This is a neighbour that you don't want to connect to. And it has FirstBlockOrdinal > block.Ordinal End Try

Go escape, you'd need to store your TryAndCatch region, and be bigger than its LastBlockOrdinal

Got It!
I'm opening a PR to fix this.

fix Fp

a4a64cf

mary-georgiou-sonarsource force-pushed the mary/lva-connect-throw branch from 8354430 to a4a64cf Compare July 16, 2024 12:46

mary-georgiou-sonarsource requested a review from sebastien-marichal July 16, 2024 12:54

sebastien-marichal reviewed Jul 16, 2024

View reviewed changes

sebastien-marichal approved these changes Jul 17, 2024

View reviewed changes

sebastien-marichal mentioned this pull request Jul 17, 2024

Fix S1854 FP: Throw should visit finally #9530

Merged

mary-georgiou-sonarsource requested review from Tim-Pohlmann and pavel-mikula-sonarsource and removed request for pavel-mikula-sonarsource July 17, 2024 09:01

Tim-Pohlmann approved these changes Jul 18, 2024

View reviewed changes

apply comments

2eb4bc4

mary-georgiou-sonarsource enabled auto-merge (squash) July 18, 2024 14:52

mary-georgiou-sonarsource merged commit a9af306 into master Jul 18, 2024

mary-georgiou-sonarsource deleted the mary/lva-connect-throw branch July 18, 2024 14:57

pavel-mikula-sonarsource reviewed Jul 19, 2024

View reviewed changes

mary-georgiou-sonarsource mentioned this pull request Jul 22, 2024

LVA: Don't connect from throw in catch to neighbouring catch #9539

Merged

	if (block.IsEnclosedIn(ControlFlowRegionKind.Catch) && block.Successors.FirstOrDefault(x => x.Semantics == ControlFlowBranchSemantics.Rethrow) is { })
	if (block.IsEnclosedIn(ControlFlowRegionKind.Catch) && block.Successors.FirstOrDefault(x => x.Semantics == ControlFlowBranchSemantics.Rethrow) is not null)

	.Where(x => x.Kind == ControlFlowRegionKind.Catch && x != block.EnclosingRegion && x.FirstBlockOrdinal > block.Ordinal)
	.Where(x => x.Kind == ControlFlowRegionKind.Catch && x.FirstBlockOrdinal > block.Ordinal)

Conversation

mary-georgiou-sonarsource commented Jul 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebastien-marichal left a comment

Choose a reason for hiding this comment

Uh oh!

Tim-Pohlmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jul 18, 2024

Quality Gate passed for 'Sonar .NET Java Plugin'

Uh oh!

sonarqubecloud bot commented Jul 18, 2024

Quality Gate passed for 'SonarAnalyzer for .NET'

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pavel-mikula-sonarsource Jul 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pavel-mikula-sonarsource Jul 19, 2024 • edited by mary-georgiou-sonarsource Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pavel-mikula-sonarsource Jul 19, 2024 •

edited

Loading

pavel-mikula-sonarsource Jul 19, 2024 •

edited by mary-georgiou-sonarsource

Loading